-
Notifications
You must be signed in to change notification settings - Fork 75
feat: aot metered cost #2168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: aot metered cost #2168
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
9b405e3 to
6e34a08
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2c7426e to
5d52991
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d45154e to
6a06185
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5235ff8 to
b125b27
Compare
|
looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Thanks for re-organizing to share code.
- I didn't catch this before, but if each of
asm_bridge_*is actually a new crate, they should not go insidearch/or even inside thevm/folder. I'd suggest makingcrates/asmand then separate folders for each crate inside.
|
|
||
| // Helper to run AOT metered-cost and compare against interpreter baseline. | ||
| #[cfg(feature = "aot")] | ||
| macro_rules! run_aot_metered_cost_and_compare { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did this need to be a macro and not just a function?
jonathanpwang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and generally helped reorganize things structurally, but please address the comment that we shouldn't have crates inside vm/src/arch. These crates should be outside, e.g. crates/asm
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Commit: 88db90d |
CodSpeed Performance ReportMerging #2168 will degrade performances by 89.25%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|---|
| ❌ | WallTime | benchmark_execute[bubblesort] |
27.4 ms | 246.3 ms | -88.86% |
| ❌ | WallTime | benchmark_execute[fibonacci_iterative] |
31.4 ms | 291.8 ms | -89.25% |
| ❌ | WallTime | benchmark_execute[fibonacci_recursive] |
45.1 ms | 396.3 ms | -88.63% |
| ❌ | WallTime | benchmark_execute[keccak256] |
33 ms | 264.5 ms | -87.53% |
| ❌ | WallTime | benchmark_execute[pairing] |
137.8 ms | 229 ms | -39.82% |
| ❌ | WallTime | benchmark_execute[quicksort] |
32 ms | 281.6 ms | -88.64% |
| ❌ | WallTime | benchmark_execute[revm_snailtracer] |
16.1 ms | 20.4 ms | -21.01% |
| ❌ | WallTime | benchmark_execute[revm_transfer] |
43.1 ms | 176.1 ms | -75.55% |
| ❌ | WallTime | benchmark_execute[sha256] |
31.1 ms | 262.9 ms | -88.18% |
| ❌ | WallTime | benchmark_execute_metered[bubblesort] |
53 ms | 280.2 ms | -81.1% |
| ❌ | WallTime | benchmark_execute_metered[fibonacci_iterative] |
72.5 ms | 324.5 ms | -77.66% |
| ❌ | WallTime | benchmark_execute_metered[fibonacci_recursive] |
101.4 ms | 465.6 ms | -78.22% |
| ❌ | WallTime | benchmark_execute_metered[keccak256] |
68.9 ms | 309.5 ms | -77.74% |
| ❌ | WallTime | benchmark_execute_metered[pairing] |
156.1 ms | 249.3 ms | -37.41% |
| ❌ | WallTime | benchmark_execute_metered[quicksort] |
60.3 ms | 321.7 ms | -81.25% |
| ❌ | WallTime | benchmark_execute_metered[revm_snailtracer] |
17.1 ms | 21.5 ms | -20.57% |
| ❌ | WallTime | benchmark_execute_metered[revm_transfer] |
64.1 ms | 201.2 ms | -68.12% |
| ❌ | WallTime | benchmark_execute_metered[sha256] |
67.1 ms | 307.2 ms | -78.15% |
Footnotes
-
No successful run was found on
feat/aot(f804542) during the generation of this report, somain(c2e376e) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩ -
42 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
We should either merge this now, or close it. |
Will merge after CI/CD finishes. Sry, didnt merge earlier cuz there were problems on main aot branch with CI/CD |
Description
Adds support for AOT Metered Cost Execution
Testing
Modified existing
fn check_aot_equivalencefunction instark_utils.rsto assert consistency between AOT and Interpreted metered cost executionAdded additional tests in
extensions/native/circuit/tests/integration_test.rsto test various scenarios surroundingmetered_costCloses INT-5255